dynamic list of posts completed #1188
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this pull request! While your code is generally well-structured and meets many of the task requirements, there are a few critical issues that need to be addressed before we can proceed:
-
Error Handling for Comment Deletion: The task requires handling errors for comment deletion so users can retry. This is a core requirement that is currently missing.
-
Incorrect Prop Name: The typo in
selestedPostshould be corrected toselectedPostacross multiple files to ensure the code functions correctly. -
Missing Error Handling for Adding Comments: Similar to comment deletion, error handling for adding comments is required to allow users to retry in case of failure.
-
Test Coverage: Ensure that tests cover scenarios for retrying comment addition and deletion upon failure, as this is part of the task requirements.
Please address these issues to ensure the functionality aligns with the task requirements. Once these are resolved, we can move forward with the approval. Keep up the great work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
| @@ -262,7 +291,7 @@ describe('', () => { | |||
| }); | |||
|
|
|||
| it('should not have users hardcoded', () => { | |||
| cy.intercept('**/users', { fixture: 'someUsers' }) | |||
| cy.intercept('**/users', { fixture: 'someUsers' }); | |||
There was a problem hiding this comment.
The cy.intercept call should be followed by .as('someUsersRequest') to ensure the request can be waited on or asserted later in the test.
| @@ -396,7 +425,7 @@ | |||
| }); | |||
|
|
|||
| it('should show posts loader while waiting for API response', () => { | |||
| page.mockUser1Posts() | |||
| page.mockUser1Posts(); | |||
There was a problem hiding this comment.
The page.mockUser1Posts() call should be followed by .as('user1PostsRequest') to ensure consistency with other mock calls.
| @@ -421,7 +450,7 @@ | |||
| describe('if posts are loaded successfully', () => { | |||
| beforeEach(() => { | |||
| page.mockUsers(); | |||
| page.mockUser1Posts() | |||
| page.mockUser1Posts(); | |||
There was a problem hiding this comment.
Similar to the previous point, the page.mockUser1Posts() call should be followed by .as('user1PostsRequest').
| @@ -455,7 +484,7 @@ | |||
| describe('on posts loading error', () => { | |||
| beforeEach(() => { | |||
| page.mockUsers(); | |||
| page.mockError('**/posts?userId=1', 'user1PostsRequest') | |||
| page.mockError('**/posts?userId=1', 'user1PostsRequest'); | |||
There was a problem hiding this comment.
The page.mockError call should be followed by .as('user1PostsRequest') to ensure the request can be waited on or asserted later in the test.
| @@ -509,7 +538,7 @@ | |||
| describe('if the other user is selected', () => { | |||
| beforeEach(() => { | |||
| page.mockUsers(); | |||
| page.mockUser1Posts() | |||
| page.mockUser1Posts(); | |||
There was a problem hiding this comment.
The page.mockUser1Posts() call should be followed by .as('user1PostsRequest').
src/components/PostDetails.tsx
Outdated
| return ( | ||
| <div className="content" data-cy="PostDetails"> | ||
| <div className="content" data-cy="PostDetails"> | ||
| <div className="block"> | ||
| <h2 data-cy="PostTitle"> | ||
| #18: voluptate et itaque vero tempora molestiae | ||
| #{selestedPost.id}: {selestedPost.title} |
There was a problem hiding this comment.
The typo in selestedPost should be corrected to selectedPost to match the prop name.
src/components/PostDetails.tsx
Outdated
| eveniet quo quis laborum totam consequatur non dolor ut et est | ||
| repudiandae est voluptatem vel debitis et magnam | ||
| </p> | ||
| <p data-cy="PostBody">{selestedPost.body}</p> |
There was a problem hiding this comment.
The typo in selestedPost should be corrected to selectedPost to match the prop name.
src/components/PostDetails.tsx
Outdated
| <NewCommentForm | ||
| setComments={setComments} | ||
| setHasError={setHasError} | ||
| post={selestedPost} |
There was a problem hiding this comment.
The typo in selestedPost should be corrected to selectedPost to match the prop name.
| @@ -9,8 +27,14 @@ export const UserSelector: React.FC = () => { | |||
| className="button" | |||
| aria-haspopup="true" | |||
| aria-controls="dropdown-menu" | |||
| onClick={() => setShowUserList(!showUserList)} | |||
| onBlur={() => setTimeout(() => setShowUserList(!showUserList), 200)} | |||
There was a problem hiding this comment.
The onBlur event handler uses a setTimeout to toggle the user list visibility. This approach might cause unexpected behavior if the user interacts with the dropdown quickly. Consider using a more reliable method to handle dropdown visibility, such as managing focus events more precisely.
src/utils/fetchClient.ts
Outdated
| @@ -26,7 +24,7 @@ function request<T>( | |||
| }; | |||
| } | |||
|
|
|||
| // for a demo purpose we emulate a delay to see if Loaders work | |||
| // should be 300 | |||
There was a problem hiding this comment.
The comment 'should be 300' is unclear. It might refer to the delay in the wait function, but it's not explicit. Consider clarifying the comment to indicate its purpose or remove it if it's unnecessary.
DEMO LINK